chore: address ProcessRunner review comments from PR #89#91
Merged
orpiske merged 2 commits intowanaku-ai:mainfrom Mar 3, 2026
Merged
Conversation
- Use redirectErrorStream(true) to prevent stderr deadlock - Use try-with-resources for BufferedReader in readOutput - Restore interrupt status before rethrowing WanakuException - Make tests cross-platform with OS detection - Add test for non-zero exit status behavior - Add test for merged stderr/stdout stream
Reviewer's GuideRefactors ProcessRunner to merge stderr into stdout, improve resource and interrupt handling, and extends tests to be cross‑platform while covering non‑zero exit codes and merged stderr/stdout behavior. Sequence diagram for ProcessRunner.runWithOutput with merged stderr/stdout and interrupt handlingsequenceDiagram
actor Caller
participant ProcessRunner
participant ProcessBuilder
participant Process
participant BufferedReader
participant WanakuException
Caller->>ProcessRunner: runWithOutput(command)
activate ProcessRunner
ProcessRunner->>ProcessBuilder: new ProcessBuilder(command)
ProcessRunner->>ProcessBuilder: redirectOutput(PIPE)
ProcessRunner->>ProcessBuilder: redirectErrorStream(true)
ProcessRunner->>ProcessBuilder: start()
ProcessBuilder-->>ProcessRunner: Process
activate Process
ProcessRunner->>Process: getInputStream()
ProcessRunner->>BufferedReader: create with InputStreamReader
activate BufferedReader
loop read lines
BufferedReader->>ProcessRunner: readLine()
alt line != null
ProcessRunner->>ProcessRunner: append line to output
else line == null
ProcessRunner->>ProcessRunner: break
end
end
BufferedReader-->>ProcessRunner: close() (try-with-resources)
deactivate BufferedReader
ProcessRunner->>Process: waitFor()
Process-->>ProcessRunner: exitCode
deactivate Process
ProcessRunner-->>Caller: output String
deactivate ProcessRunner
%% Error path: IOException
Caller->>ProcessRunner: runWithOutput(command) (I/O error)
activate ProcessRunner
ProcessRunner->>WanakuException: new WanakuException(IOException)
ProcessRunner-->>Caller: throw WanakuException
deactivate ProcessRunner
%% Error path: InterruptedException
Caller->>ProcessRunner: runWithOutput(command) (Interrupted)
activate ProcessRunner
ProcessRunner->>ProcessRunner: Thread.currentThread().interrupt()
ProcessRunner->>WanakuException: new WanakuException(InterruptedException)
ProcessRunner-->>Caller: throw WanakuException
deactivate ProcessRunner
Updated class diagram for ProcessRunner with stderr/stdout merge and resource handlingclassDiagram
class ProcessRunner {
- ProcessRunner()
+ static String runWithOutput(String[] command)
- static String readOutput(Process process)
+ static void run(File directory, String[] command)
+ static void run(File directory, Map environmentVariables, String[] command)
}
class WanakuException {
+ WanakuException(Throwable cause)
}
class ProcessBuilder {
+ ProcessBuilder(String[] command)
+ ProcessBuilder redirectOutput(Redirect redirect)
+ ProcessBuilder redirectErrorStream(boolean redirect)
+ Process start()
}
class Process {
+ InputStream getInputStream()
+ int waitFor()
}
class BufferedReader {
+ BufferedReader(Reader in)
+ String readLine()
+ void close()
}
class Thread {
+ static void currentThread_interrupt()
}
class Logger {
+ void info(String message)
+ void error(String message, String details, Throwable throwable)
}
ProcessRunner ..> WanakuException : throws
ProcessRunner ..> ProcessBuilder : uses
ProcessRunner ..> Process : uses
ProcessRunner ..> BufferedReader : uses
ProcessRunner ..> Thread : restores_interrupt
ProcessRunner ..> Logger : logs
ProcessBuilder ..> Process : creates
Process ..> BufferedReader : provides_InputStream
Thread <|.. Thread : currentThread_interrupt
class Redirect
class File
class Map
class InputStream
class Reader
ProcessBuilder ..> Redirect : uses
Process ..> InputStream : returns
BufferedReader ..> Reader : wraps
ProcessRunner ..> File : uses
ProcessRunner ..> Map : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
readOutput, consider explicitly specifying a charset forInputStreamReader(e.g.,StandardCharsets.UTF_8) instead of relying on the platform default to avoid surprises across different environments. - The Windows/non-Windows command selection logic in the tests is duplicated across multiple test methods; consider extracting a small helper (e.g.,
runEchoCommand/runCommand(String unixCmd, String windowsCmd)) to keep the tests more concise and avoid repetition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `readOutput`, consider explicitly specifying a charset for `InputStreamReader` (e.g., `StandardCharsets.UTF_8`) instead of relying on the platform default to avoid surprises across different environments.
- The Windows/non-Windows command selection logic in the tests is duplicated across multiple test methods; consider extracting a small helper (e.g., `runEchoCommand` / `runCommand(String unixCmd, String windowsCmd)`) to keep the tests more concise and avoid repetition.
## Individual Comments
### Comment 1
<location path="capabilities-common/src/main/java/ai/wanaku/capabilities/sdk/common/ProcessRunner.java" line_range="45" />
<code_context>
- String line;
- while ((line = reader.readLine()) != null) {
- output.append(line).append("\n");
+ try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) {
+ StringBuilder output = new StringBuilder();
+ String line;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider specifying an explicit charset when reading process output.
`InputStreamReader` without a charset uses the platform default, which may differ from the subprocess encoding and vary between environments. Please pass an explicit charset (e.g., `new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8)`) to ensure consistent decoding across platforms.
Suggested implementation:
```java
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
```
```java
try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) {
```
</issue_to_address>
### Comment 2
<location path="capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java" line_range="11-13" />
<code_context>
public class ProcessRunnerTest {
+ private static boolean isWindows() {
+ return System.getProperty("os.name").toLowerCase().contains("win");
+ }
+
@Test
</code_context>
<issue_to_address>
**suggestion:** Consider extracting OS-specific command construction to a helper to avoid duplication across tests
`runWithOutput_returnsCommandOutput`, `runWithOutput_nonZeroExitDoesNotThrow`, and `runWithOutput_capturesStderrViaMergedStream` all duplicate the `if (isWindows()) { cmd.exe ... } else { sh ... }` logic. A small helper like `private static String[] shellCommand(String script)` that returns the OS‑specific command array would centralize this logic so future changes to OS detection or shell invocation only need to be made in one place.
Suggested implementation:
```java
private static boolean isWindows() {
return System.getProperty("os.name").toLowerCase().contains("win");
}
private static String[] shellCommand(String script) {
if (isWindows()) {
return new String[]{"cmd.exe", "/c", script};
} else {
return new String[]{"sh", "-c", script};
}
}
```
```java
@Test
void runWithOutput_returnsCommandOutput() {
String[] command = shellCommand("echo hello");
String output = ProcessRunner.runWithOutput(command);
assertNotNull(output, "Output should not be null");
```
There are likely similar OS-specific command constructions in the tests `runWithOutput_nonZeroExitDoesNotThrow` and `runWithOutput_capturesStderrViaMergedStream`. For each of those tests, replace the duplicated `if (isWindows()) { cmd.exe ... } else { sh ... }` blocks by constructing the command using `shellCommand("<appropriate script>")` and then calling `ProcessRunner.runWithOutput(command)` (or the relevant ProcessRunner method) with the resulting array.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
capabilities-common/src/main/java/ai/wanaku/capabilities/sdk/common/ProcessRunner.java
Outdated
Show resolved
Hide resolved
capabilities-common/src/test/java/ai/wanaku/capabilities/sdk/common/ProcessRunnerTest.java
Outdated
Show resolved
Hide resolved
- Use StandardCharsets.UTF_8 in InputStreamReader for consistent decoding - Extract shellCommand() helper in tests to remove OS-detection duplication
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
redirectErrorStream(true)to merge stderr into stdout, preventing deadlock when subprocess writes heavily to stderrBufferedReaderinreadOutputto prevent resource leaksThread.currentThread().interrupt()before rethrowingWanakuExceptiononInterruptedExceptionTest plan
mvn verifypassesProcessRunnerTesttests passSummary by Sourcery
Improve ProcessRunner robustness and portability while expanding its test coverage.
Bug Fixes:
Enhancements:
Tests: